feat(mcp): add duplicate_dashboard tool#40959
Conversation
Adds a duplicate_dashboard MCP tool that clones an existing dashboard via CopyDashboardCommand. The source dashboard can be identified by numeric ID, UUID, or slug. By default the copy references the same charts; duplicate_slices=true deep-copies every chart into new objects owned by the caller. The tool builds the required json_metadata payload (source metadata plus a positions key from position_json), mirroring what the frontend "Save as" flow sends to the /copy/ endpoint. The new title is sanitized for XSS, and the tool is excluded from MCP response caching.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #40959 +/- ##
==========================================
- Coverage 64.19% 64.09% -0.10%
==========================================
Files 2655 2659 +4
Lines 143925 144527 +602
Branches 33181 33288 +107
==========================================
+ Hits 92386 92636 +250
- Misses 49919 50251 +332
- Partials 1620 1640 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Code Review Agent Run #e866e5Actionable Suggestions - 0Additional Suggestions - 2
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Address review feedback: emit ctx.warning on not-found / access-denied early returns for observability parity, and add dedicated schema unit tests for DuplicateDashboardRequest sanitization and DuplicateDashboardResponse error wrapping.
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| class TestDuplicateDashboardRequestTitleSanitization: | ||
| """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title.""" | ||
|
|
||
| def test_plain_title_passes_without_warning(self) -> None: |
There was a problem hiding this comment.
Suggestion: Add a short docstring to this newly added test function to comply with the rule requiring docstrings on new functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added test method in the PR and it has no docstring. The rule requires new Python functions and classes to include docstrings, so the suggestion correctly identifies a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 635:635
**Comment:**
*Custom Rule: Add a short docstring to this newly added test function to comply with the rule requiring docstrings on new functions.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Addressed in 5e9923e — added a docstring to this test method.
| assert req.dashboard_title == "Regional Copy" | ||
| assert req.sanitization_warnings == [] | ||
|
|
||
| def test_title_accepts_aliases(self) -> None: |
There was a problem hiding this comment.
Suggestion: Add an inline docstring describing the alias behavior being validated so the new function is documented. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This newly added test method does not include a docstring. That matches the stated rule requiring docstrings for new functions, so this is a verified violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 640:640
**Comment:**
*Custom Rule: Add an inline docstring describing the alias behavior being validated so the new function is documented.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Addressed in 5e9923e — added a docstring to this test method.
| req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name") | ||
| assert req.dashboard_title == "From Name" | ||
|
|
||
| def test_script_only_title_is_rejected(self) -> None: |
There was a problem hiding this comment.
Suggestion: Add a concise docstring explaining the rejection case being tested to satisfy the new-function docstring requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The method is newly added and has no docstring. Since the rule says new Python functions should be documented inline, this is a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 644:644
**Comment:**
*Custom Rule: Add a concise docstring explaining the rejection case being tested to satisfy the new-function docstring requirement.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Addressed in 5e9923e — added a docstring to this test method.
| class TestDuplicateDashboardResponse: | ||
| """Serialization and error sanitization for DuplicateDashboardResponse.""" | ||
|
|
||
| def test_defaults(self) -> None: |
There was a problem hiding this comment.
Suggestion: Add a brief docstring for this new test method so the function is documented inline. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new test method lacks a docstring. The custom rule explicitly requires docstrings on newly added Python functions, so the suggestion is verified.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 675:675
**Comment:**
*Custom Rule: Add a brief docstring for this new test method so the function is documented inline.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Addressed in 5e9923e — added a docstring to this test method.
| assert resp.error is None | ||
| assert resp.warnings == [] | ||
|
|
||
| def test_error_is_wrapped_for_llm_context(self) -> None: |
There was a problem hiding this comment.
Suggestion: Add a docstring describing the error-wrapping assertion to document this newly added function. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added test method has no docstring in the final file state. That directly violates the rule for newly added Python functions/classes to be documented inline.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
**Line:** 683:683
**Comment:**
*Custom Rule: Add a docstring describing the error-wrapping assertion to document this newly added function.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Addressed in 5e9923e — added a docstring to this test method.
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mock_auth(): |
There was a problem hiding this comment.
Suggestion: Add an explicit return type annotation to mock_auth so the new fixture function is fully typed according to the rule for new Python code. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly added Python function in a new test file, and it lacks a return type annotation. The custom rule requires new Python code to be fully typed, so the suggestion correctly identifies a real violation.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py
**Line:** 60:60
**Comment:**
*Custom Rule: Add an explicit return type annotation to `mock_auth` so the new fixture function is fully typed according to the rule for new Python code.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Addressed in 5e9923e — added -> Iterator[MagicMock] return type to the fixture.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| try: | ||
| positions = json.loads(source.position_json or "{}") | ||
| except (json.JSONDecodeError, TypeError): | ||
| positions = {} | ||
| if not isinstance(positions, dict): | ||
| positions = {} | ||
|
|
||
| metadata["positions"] = positions |
There was a problem hiding this comment.
Suggestion: Falling back to an empty positions object when position_json is missing/invalid will silently drop all charts in the copied dashboard. DashboardDAO.copy_dashboard/set_dash_metadata rebuilds dashboard slices from positions, so {} produces an empty slice list even when the source dashboard had charts. Fail fast on invalid/missing layout or build a positions map from source charts before calling the copy command. [logic error]
Severity Level: Major ⚠️
- ❌ MCP duplicate_dashboard can create empty dashboards unexpectedly.
- ⚠️ Source dashboards with bad layout copy without any charts.
- ⚠️ LLM workflows see misleading empty dashboard copies.Steps of Reproduction ✅
1. Create or identify a dashboard object with charts but invalid or missing layout JSON:
in `superset/models/dashboard.py:131-147` the `Dashboard` model has `position_json` and
`slices` fields; use a DB migration or shell to set `position_json` to `NULL` or a
non-JSON string for a dashboard that still has non-empty `slices`.
2. Call the MCP tool `duplicate_dashboard` defined in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:131-144`, passing that
dashboard's ID/UUID/slug via `DuplicateDashboardRequest.dashboard_id`.
3. Inside `duplicate_dashboard`, `_build_copy_payload` is invoked at
`duplicate_dashboard.py:187-189`, which parses `source.position_json` at lines `67-72`;
because `position_json` is `NULL`/invalid, the `except` block and type check coerce
`positions` to `{}`, then `metadata["positions"] = positions` at line 74 ensures the
copied payload always contains an empty `positions` dict.
4. `CopyDashboardCommand.run()` in `superset/commands/dashboard/copy.py:11-14` calls
`DashboardDAO.copy_dashboard`, which in turn calls `DashboardDAO.set_dash_metadata(dash,
metadata, old_to_new_slice_ids)` at `superset/daos/dashboard.py:43-44`; since
`data.get("positions")` is `{}` (non-None), `set_dash_metadata` at `dashboard.py:22-36`
computes `slice_ids = []`, sets `dashboard.slices = current_slices` where `current_slices`
is `[]`, and writes the new dashboard with no charts even though the source had charts,
causing the duplicated dashboard to lose all charts.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/duplicate_dashboard.py
**Line:** 67:74
**Comment:**
*Logic Error: Falling back to an empty `positions` object when `position_json` is missing/invalid will silently drop all charts in the copied dashboard. `DashboardDAO.copy_dashboard`/`set_dash_metadata` rebuilds dashboard slices from `positions`, so `{}` produces an empty slice list even when the source dashboard had charts. Fail fast on invalid/missing layout or build a positions map from source charts before calling the copy command.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Good catch — fixed in 5e9923e. The tool now detects when the source has charts but its layout maps none (missing/invalid position_json) and fails fast with a clear error, instead of letting set_dash_metadata rebuild the copy with zero slices and silently produce an empty dashboard. Added a regression test.
| info = DashboardInfo( | ||
| id=dashboard.id, | ||
| dashboard_title=dashboard.dashboard_title, | ||
| slug=dashboard.slug, | ||
| description=dashboard.description, | ||
| published=dashboard.published, | ||
| created_on=dashboard.created_on, | ||
| changed_on=dashboard.changed_on, | ||
| uuid=str(dashboard.uuid) if dashboard.uuid else None, | ||
| url=dashboard_url, | ||
| chart_count=len(dashboard.slices), | ||
| tags=[ | ||
| obj | ||
| for tag in getattr(dashboard, "tags", []) | ||
| if (obj := serialize_tag_object(tag)) is not None | ||
| ], | ||
| charts=[ | ||
| obj | ||
| for chart in getattr(dashboard, "slices", []) | ||
| if ( | ||
| obj := serialize_chart_summary( | ||
| chart, | ||
| include_data_model_metadata=include_data_model_metadata, | ||
| ) | ||
| ) | ||
| is not None | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Suggestion: The tool returns dashboard, chart, and tag text fields without LLM-context sanitization, unlike the standard dashboard serializers. Since these fields are user-controlled, this can surface prompt-injection payloads directly in MCP structured responses. Sanitize the response payload (or reuse the existing sanitized dashboard serializer path) before returning it. [security]
Severity Level: Critical 🚨
- ❌ MCP duplicate_dashboard returns unsanitized dashboard text to LLM.
- ❌ User dashboards can inject prompts into MCP tool output.
- ⚠️ Inconsistent with sanitized dashboard serializers in schemas module.Steps of Reproduction ✅
1. In Superset, create or edit a dashboard so that `dashboard_title` or `description` on
the `Dashboard` model (`superset/models/dashboard.py:131-143`) contains a prompt-injection
payload (for example, instructions to the LLM), and add charts and tags whose
`slice_name`/`description` and `Tag.name`/`Tag.description` also contain crafted text.
2. From an MCP client, call the `duplicate_dashboard` tool implemented in
`superset/mcp_service/dashboard/tool/duplicate_dashboard.py:131-144`, passing this
dashboard's identifier in `DuplicateDashboardRequest.dashboard_id`; the tool copies the
dashboard and then calls `_serialize_new_dashboard(new_dashboard)` at
`duplicate_dashboard.py:212`.
3. `_serialize_new_dashboard` at `duplicate_dashboard.py:84-118` builds a `DashboardInfo`
instance directly from ORM attributes: `dashboard_title`, `slug`, `description`, `css`,
and iterates `dashboard.tags` using `serialize_tag_object`
(`superset/mcp_service/dashboard/schemas.py:137-147`) and `dashboard.slices` using
`serialize_chart_summary` (`superset/mcp_service/dashboard/schemas.py:5-29`), all without
passing the data through `_sanitize_dashboard_info_for_llm_context`.
4. The populated `DashboardInfo` is returned to the MCP client inside
`DuplicateDashboardResponse` at `duplicate_dashboard.py:235-240` with `dashboard=info`;
unlike other dashboard serializers such as `dashboard_serializer` and
`serialize_dashboard_object` in `superset/mcp_service/dashboard/schemas.py:41-120`, which
wrap their `DashboardInfo` via `_sanitize_dashboard_info_for_llm_context` (lines 43-38 in
that file) to call `sanitize_for_llm_context` and `escape_llm_context_delimiters`, this
path leaves all user-controlled fields unsanitized, exposing prompt-injection payloads
directly in structured MCP responses consumed by the LLM.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/dashboard/tool/duplicate_dashboard.py
**Line:** 90:117
**Comment:**
*Security: The tool returns dashboard, chart, and tag text fields without LLM-context sanitization, unlike the standard dashboard serializers. Since these fields are user-controlled, this can surface prompt-injection payloads directly in MCP structured responses. Sanitize the response payload (or reuse the existing sanitized dashboard serializer path) before returning it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Fixed in 5e9923e. The response now routes the new dashboard's DashboardInfo through _sanitize_dashboard_info_for_llm_context (same path as dashboard_serializer/serialize_dashboard_object), so title/description/chart/tag text is wrapped as untrusted before reaching LLM context. Added a test asserting the response title comes back LLM-context-wrapped.
Code Review Agent Run #fe4939Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Address review feedback on duplicate_dashboard: - Route the new dashboard's response through _sanitize_dashboard_info_for_llm_context so user-controlled title, description, chart, and tag text are wrapped as untrusted before reaching LLM context, matching the standard dashboard serializers. - Fail fast when the source has charts but its layout maps none: an empty/invalid position_json would otherwise make set_dash_metadata rebuild the copy with zero slices, silently producing an empty copy. - Add tests for both paths and docstrings/type hints on new test code.
Code Review Agent Run #9d6b93Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
Adds a new
duplicate_dashboardMCP tool to the Superset MCP service. The canonical AI workflow this enables: "create a regional/staging variant of this dashboard" — clone a template dashboard, then edit the copy.Design:
CopyDashboardCommand(the same backend used by the frontend "Save as" flow on/api/v1/dashboard/<id>/copy/).DashboardDAO.get_by_id_or_slug, which applies dashboard access filters).DashboardCopySchema's copy data contract requiresjson_metadata; the tool builds it server-side from the source dashboard's current state (json_metadataplus apositionskey fromposition_json), mirroring exactly what the frontend "Save as" sends.json_metadatais intentionally not exposed as a tool parameter.duplicate_slices: bool = false— when true, every chart on the source dashboard is deep-copied into a new chart object owned by the caller; when false the copy references the same charts.generate_dashboard(nh3-basedsanitize_user_input), with a warning surfaced when content is stripped and a hard error when the title sanitizes to nothing.DASHBOARD_RBAC), and copy failures; errors are wrapped for LLM-context safety.mutatetool (class_permission_name="Dashboard",method_permission_name="write") and added toMCP_CACHE_CONFIG["excluded_tools"]so responses are never cached.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — MCP service tool, no UI changes.
TESTING INSTRUCTIONS
duplicate_dashboardwith{"dashboard_id": <existing id|uuid|slug>, "dashboard_title": "My Copy"}— verify a new dashboard is created referencing the same charts, and the returned URL opens it."duplicate_slices": true— verify the new dashboard contains new chart objects (different chart IDs) with the same layout.pytest tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.pyADDITIONAL INFORMATION